Repository navigation
Fix the follow-ups from the #87–#89 review (#91–#96) - #97
Closed
gravitypdf-bot wants to merge 7 commits into
Closed
gravitypdf-bot wants to merge 7 commits into
gravitypdf-bot wants to merge 7 commits into
Conversation
initializeFromError() passed the error number as the message and the warning text, cast to int, as the code. Pass them in the constructor's order, and throw static so IOException no longer needs its own copy. Fixes #91 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
writeHTML() installed ParseException's error handler where the other writers install IOException's. testFailWriteHTML() called writeXML(), so the failure path went untested; it now calls writeHTML(). Fixes #92 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Seven blocks installed initializeFromError() as the error handler and restored it only from catch (QueryPath\Exception), so any other throwable left the handler installed. Route them all through DOM::withErrorHandler(), which restores it in finally. Fixes #95 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tag() read tagName on whatever node came first, so a text, comment or processing instruction node from contents() raised an "Undefined property" warning and returned NULL. Return an empty string instead, matching the empty match set. Fixes #93. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QPXML::createElement() looked an element name's prefix up on the owner document, which only sees declarations on the document element, so a prefix declared on the matched element or an ancestor below the root threw "Undefined namespace". Look it up on the element instead; root declarations are in scope there too. The foreach returned on its first pass, so only the first match was ever used. Use get(0) so the code says so. Fixes #94 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The three getters were copies of the same loop that had drifted: cdata() used === and returned null explicitly, while comment() and pi() used == and fell off the end. Move the loop into a private firstChildText() helper, keeping pi()'s filter on the processing instruction's target, and give pi() a docblock to match the other two. No behaviour change. Tests pin down the not-found, first-across-matches and child-only cases for all three getters. Fixes #96 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Decide the file parser before entering the error handler, use expectException*() and a data provider in the new tests, drop the rethrow-only try/catch blocks in the write-failure tests, and trim comments that restated the code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
============================================
+ Coverage 89.84% 90.57% +0.73%
+ Complexity 1430 1420 -10
============================================
Files 26 25 -1
Lines 3189 3162 -27
============================================
- Hits 2865 2864 -1
+ Misses 324 298 -26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gravitypdf-bot
added a commit
that referenced
this pull request
Oct 25, 2026
attr(), addClass(), removeClass(), css(), hasClass(), hasAttr() and
removeAttr() called element-only DOM methods on every match, so a text,
comment, CDATA or processing instruction node in the set (from contents(),
for example) was a fatal Error.
Add DOM::elements(), which returns the element matches in order, and route
those methods through it. As in jQuery, setters skip non-elements and still
return the DOMQuery, and the predicates consider only the elements. The
attr() getters keep reading the first match and return NULL when it is not
an element, as they do for an empty set; attr('nodeType') still reports the
first match's type, whatever it is.
tag() is left for #97.
Fixes #98
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gravitypdf-bot
added a commit
that referenced
this pull request
Oct 26, 2026
attr(), addClass(), removeClass(), css(), hasClass(), hasAttr() and
removeAttr() called element-only DOM methods on every match, so a text,
comment, CDATA or processing instruction node in the set (from contents(),
for example) was a fatal Error.
Add DOM::elements(), which returns the element matches in order, and route
those methods through it. As in jQuery, setters skip non-elements and still
return the DOMQuery, and the predicates consider only the elements. The
attr() getters keep reading the first match and return NULL when it is not
an element, as they do for an empty set; attr('nodeType') still reports the
first match's type, whatever it is.
tag() is left for #97.
Fixes #98
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the follow-ups found while reviewing #87–#89.
Fixes #91, fixes #92, fixes #93, fixes #94, fixes #95, fixes #96
Changes
IOException/ParseExceptionraised from a PHP warning lose the warning text — message is the error number #91: exceptions now keep the PHP warning text.ParseException::initializeFromError()passed its arguments in the wrong order, so the message was the error number and the code was0. It now throwsnew static($errstr, $errno, ...), and the identical override inIOExceptionhas been removed. This changes the exception message, so the CHANGELOG notes it for anyone matching on the old text.writeHTML()throwsParseExceptioninstead ofIOExceptionwhen the file cannot be written #92:writeHTML()throwsIOExceptionwhen the file can't be written, like the otherwrite*()methods.testFailWriteHTML()now callswriteHTML(); it used to callwriteXML().finally-based helper #95: one error-handler helper. The sevenset_error_handler/catch/restore_error_handlerblocks inDOMandDOMQuerynow all go throughDOM::withErrorHandler(), which restores the handler infinally. Before, any throwable that wasn't aQueryPath\Exception(aTypeError, or an exception from a stream wrapper) left QueryPath's handler installed.tag()warns and returns NULL when the first match is not an element #93:tag()returns''when the first match isn't an element, for example a text node fromcontents(). That matches what it already returns for an empty set.QPXML::createElement()only resolves prefixes declared on the document element #94:QPXML::createElement()resolves the prefix against the first matched element, not the document, so a prefix declared below the root now works. Theforeachthat only ever ran once is replaced withget(0).QPXML::cdata(),comment()andpi()getters #96:cdata(),comment()andpi()share one private helper,firstChildText().pi()now has a full docblock. There is no behaviour change, so this one has no CHANGELOG entry.Tests
Each fix has a test that fails on
mainand passes on this branch:IOException/ParseExceptionraised from a PHP warning lose the warning text — message is the error number #91: the exception message contains the warning text, and the code isE_WARNING.writeHTML()throwsParseExceptioninstead ofIOExceptionwhen the file cannot be written #92:writeHTML()to an unwritable path throwsIOException.finally-based helper #95:testErrorHandlerRestoredAfterForeignThrowableuses a newThrowingStreamWrapperfixture to raise aRuntimeExceptioninsidewriteXML(),writeHTML(),writeHTML5()and a load with a stream context. It then checks that the previous error handler is back.parseXMLString()andprepareInsert()have no such test, because there's no reliable way to make them throw a non-QueryPath exception.tag()warns and returns NULL when the first match is not an element #93:tag()on a match set whose first node is a text, comment, processing-instruction or element node.QPXML::createElement()only resolves prefixes declared on the document element #94: prefixes declared below the root, on the root, and out of scope; only the first match is used; an empty match set.QPXML::cdata(),comment()andpi()getters #96: before the refactor, I added tests for the cases with nothing to find and for the first hit across several matches, and ran them green on the old code.The full suite (415 tests),
composer run lintandcomposer run lint:min-phppass locally on PHP 8.5. CI covers 7.1–8.4.Not in this PR
Other methods still call element-only DOM methods on a match without checking its type, so they can fail on a
contents()result the waytag()did: theattr()getter and setter,addClass(),removeClass(),css(),hasClass(),hasAttr()andremoveAttr(). That's the same class of bug as #93, but wider in scope, and probably best fixed with one element-only accessor onDOM. Tracked in #98.🤖 Generated with Claude Code